Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for show_complex interaction with scalebar dict #690

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

edwinsupple
Copy link
Contributor

See #673 and #689. show_complex incorrectly assigned None to figsize when popping it from the kwargs resulting in broken plotting due to interaction with scalebar, it now assigns a default figsize as is typical for other show type functions. I did not find the same issue in the other show functions. @smribet please let me know if you have a case where this is still broken.

@gvarnavi
Copy link
Member

gvarnavi commented Oct 1, 2024

Thanks @edwinsupple for the PR.

If I recall correctly #673 broke a number of other downstream visualization functions used in the phase module (adding multiple scalebars etc). I can try and reproduce those bugs later this week.

Taking a step back, it's not obvious to me popping kwargs is the way to go? A simpler approach for #672 could perhaps be to check whether the user passed any scalebar options, and if so and the array to be plotted has their own calibrations, to simply extract the array (bf.data) and plot that?

@edwinsupple
Copy link
Contributor Author

@gvarnavi there are a lot of instances where figsize gets set via something like figsize = kwargs.pop("figsize", (5, 5)). For example _visualize_last_iteration and _visualize_all_iterations in dpc.py.

Looking at this more, I think the issue may be that my initial PR did not handle None correctly. I was focused on the first line in the docstring about adding a scalebar when a DiffractionSlice or RealSlice is passed and missed the bit about not adding a scalebar when an array is passed. scalebar=None was changed to scalebar={} at the start of show and then lines 757-772 do happen when they shouldn't.

I will go back and work on this more, if you have a test case or two I can try let me know.

@edwinsupple edwinsupple marked this pull request as draft October 2, 2024 14:54
@gvarnavi
Copy link
Member

gvarnavi commented Oct 2, 2024

Indeed, I added those haha -- the purpose of that is to allow compatibility with different function call structures and impose opinionated defaults in the downstream functions.

To be quite honest, the objection here is on the way kwargs get handled in show, which of-course predates #673. We have had many many discussions internally that show has become too unwieldy and needs to be refactored..

Which is why the suggestion was a minimally-invasive "hack" to achieve your effect w/o touching the entangled kwargs mess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants